Skip to content

Fix card loading (WIP) - #821

Merged
joshsmith merged 1 commit into
developfrom
811-fix-card-loading
Dec 2, 2016
Merged

Fix card loading (WIP)#821
joshsmith merged 1 commit into
developfrom
811-fix-card-loading

Conversation

@joshsmith

Copy link
Copy Markdown
Contributor

What's in this PR?

WIP

@joshsmith

joshsmith commented Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

Various states:

new card processing - show the form and “Processing…” button (action on form)
existing card processing - show the card and “Processing…” button (action on container)
no card processing - show the form and “Processing...” button (action on form)
new card not processing - show the card and “Donate” button (action on form)
existing card not processing - show the card and “Donate" button (action on container)
no card not processing - show the form and “Donate” button (action on form)

The problem is with the bolded state right now. The way that we have it, we render two buttons in this state, one on donation/credit-card because {{#if (and card (not isProcessing))}} evaluates as true and one on donation-container because it {{#if card}} evaluates as true.

@noslouch

noslouch commented Dec 2, 2016

Copy link
Copy Markdown

which is the desired button to show? the button on donation/donation-container or donation/credit-card?

@begedin begedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good, so approving, but there's one comment that we might want to consider.

projectTitle: null,
wasNewCard: true,

isNewCard: empty('card.id'),

@begedin begedin Dec 2, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone might wonder why this isn't simply alias(cad.isNew), not realizing it's because card might be null to begin with. Maybe a comment would help?

At least, that's my assumption here. Maybe card.isNew would actually work? Does init get called if the component is not rendered?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@begedin this one is tricky because it's an ObjectProxy which has content: null if there is none. This was the least hacky way I could think to deal with this.

Should definitely comment, but would prefer a better fix.

I'm not super satisfied with my implementation over all, but hey: it works.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a request for a better fix in the comment. Maybe someone gets inspired.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@begedin can you make an issue refing this PR for us to fix? And put under this milestone, I guess.

@joshsmith
joshsmith force-pushed the 811-fix-card-loading branch from e8fd2a9 to 7fd5da0 Compare December 2, 2016 07:18
@joshsmith joshsmith added this to the Improve Donations milestone Dec 2, 2016
@joshsmith
joshsmith merged commit abecd90 into develop Dec 2, 2016
@joshsmith
joshsmith deleted the 811-fix-card-loading branch December 2, 2016 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants